bench(resolver): add napi-sync scenario (block_on per resolve)#276
bench(resolver): add napi-sync scenario (block_on per resolve)#276stormslowly wants to merge 1 commit into
Conversation
The existing scenarios drive resolution inside a long-lived tokio runtime with the whole batch in one async block (or a JoinSet). The most common real consumer — @rspack/resolver's NAPI `.sync()` API — instead runs one `Handle::current().block_on(resolve(...))` per call. Add a scenario that mirrors that per-call block_on so the bench reflects how synchronous consumers actually invoke the resolver.
Merging this PR will not alter performance
Performance Changes
Comparing 
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request adds a new Criterion benchmark case to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20f9dbc286
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let runner = runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build() | ||
| .expect("failed to create tokio runtime"); |
There was a problem hiding this comment.
For native npm users, napi-rs 3.9.0 creates its default runtime with Builder::new_multi_thread(), and within_runtime_if_available enters that runtime before Handle::current().block_on; this benchmark instead uses a current-thread runtime. This changes scheduling and tokio::fs/spawn_blocking behavior, so the scenario labeled as mirroring NAPI .sync() can produce performance results that do not represent the path it is intended to track. Construct the same multi-thread runtime as napi-rs (and ideally enter it as the wrapper does) before measuring per-call block_on.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a new Criterion benchmark scenario intended to reflect the @rspack/resolver NAPI “sync” usage shape by running a separate block_on per resolve (instead of batching all resolves into one async block), making the per-call runtime boundary cost explicit in perf tracking.
Changes:
- Add a new
napi-sync (block_on per resolve)benchmark scenario inbenches/resolver.rs. - Reuse the existing ~1000 real npm-specifier dataset and per-iteration cache clearing, consistent with existing resolver benchmarks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Models the `@rspack/resolver` (NAPI) `.sync()` entry point — the common npm | ||
| // path — which runs one `Handle::current().block_on(resolve(...))` per call. | ||
| // Unlike `single-thread` (a single async block over the whole batch), every | ||
| // resolve enters and exits the runtime on its own, matching how a synchronous | ||
| // consumer (rspack injecting the resolver, or a direct `.sync()` caller) | ||
| // actually invokes it. |
| }, | ||
| |_| { | ||
| for (path, request) in data { | ||
| let _ = runner.block_on(rspack_resolver.resolve(path, request)); |
Why
The
resolverbench drives every scenario inside a long-lived tokio runtimewith the whole 1000-specifier batch in a single async block (or a
JoinSet).That shape was tuned around
tokio::fs'sspawn_blocking, but it is not howthe resolver is actually consumed:
@rspack/resolver(NAPI).sync()— the common npm entry point — runsone
Handle::current().block_on(resolve(...))per call.resolver.resolve(dir, req).awaitonce per module.Batching the whole set into one async block hides the per-call runtime
entry/exit that real callers pay.
What
Add a
napi-sync (block_on per resolve)scenario that mirrors the NAPI.sync()path: each resolve is its ownblock_on, over the same ~1000real npm specifiers. Existing scenarios are untouched, so their CodSpeed
history is unaffected.
This is additive and complements (does not depend on) the
std::fschange in#275 — the per-call
block_onpattern is exactly where droppingtokio::fs'sper-syscall
spawn_blockingpays off most for npm users.What it measures (devbox callgrind)
Running this new scenario before/after the
std::fschange in #275 confirms ittracks the real npm-sync path and captures the full win:
The absolute counts sit right on top of the batched
single-threadscenario(189.5M vs 188.3M cycles on tokio::fs), so the per-call
block_onoverhead isnegligible — i.e. npm
.sync()users get the same ~25% speedup, not more andnot less. The scenario earns its place by making that real-usage shape explicit
rather than by surfacing a different number.